refactor: consolidate Runner and app ScriptRunner#9617
Conversation
… handling (#9610) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
3 issues found across 10 files
Architecture diagram
sequenceDiagram
participant Client as App/Cell API
participant by_kwargs as by_kwargs module
participant Evaluator as Evaluator
participant Executor as DefaultExecutor
participant Scheduler as SequentialScheduler
participant Graph as DirectedGraph
Note over Client,Graph: NEW: Cell.run(**kwargs) public API path
Client->>by_kwargs: run_cell_sync(graph, cellId, kwargs)
by_kwargs->>Graph: cells[cellId].is_coroutine()
alt Cell is coroutine
by_kwargs->>by_kwargs: Raise RuntimeError
else Sync cell
by_kwargs->>by_kwargs: _validate_kwargs(cell, kwargs)
by_kwargs->>Graph: Get ancestor closure (pruned by substitutions)
by_kwargs->>by_kwargs: topological_sort(graph, ancestors)
by_kwargs->>Evaluator: evaluate_sync(ancestorCell, glbls)
Evaluator->>Executor: execute_cell(ancestorCell, glbls)
loop For each ancestor
Executor-->>Evaluator: glbls updated
end
by_kwargs->>by_kwargs: _substitute_refs(cell, glbls, kwargs)
by_kwargs->>Evaluator: evaluate_sync(targetCell, glbls)
Evaluator->>Executor: execute_cell(targetCell, glbls)
Executor-->>Evaluator: output
by_kwargs-->>Client: (output, definitions)
end
Note over Client,Scheduler: NEW: ScriptRunner consolidated flow
Client->>Scheduler: __init__(cells_to_run, graph)
Scheduler->>Scheduler: Pop next cell in order
loop For each cell
Scheduler-->>Client: pop_cell()
alt Cell is cancelled
Client->>Scheduler: cancelled(cid) == True
Client->>Client: Skip execution
else Cell not cancelled
Client->>Evaluator: evaluate_sync(cell, glbls)
Evaluator->>Executor: execute_cell(cell, glbls)
alt Execution succeeds
Executor-->>Evaluator: output
Evaluator-->>Client: RunResult(output=..., exception=None)
Client->>Client: _handle_run_result: record output
else Execution raises MarimoRuntimeException
Evaluator-->>Client: RunResult(exception=MarimoRuntimeException)
Client->>Client: unwrap_user_exception()
alt MarimoStopError
Client->>Client: Record stop output
Client->>Scheduler: cancel(cid)
Scheduler->>Scheduler: Mark descendants cancelled
else Other exception
Client->>Client: Re-raise exception
end
end
end
end
Client-->>Client: Return (outputs, glbls)
Note over Evaluator,Executor: NEW: evaluate_sync & evaluate_interruptible added
Client->>Evaluator: evaluate_interruptible(cell, glbls)
alt Cell is coroutine & main thread
Evaluator->>Evaluator: Create future, wrap with SIGINT handler
Evaluator->>Evaluator: asyncio.ensure_future(evaluate(...))
alt SIGINT received
Evaluator->>Evaluator: Cancel future → CancelledError
else Normal completion
Evaluator-->>Client: RunResult
end
else Sync cell or not main thread
Evaluator->>Evaluator: evaluate(cell, glbls) directly
Evaluator-->>Client: RunResult
end
Note over Evaluator,Executor: CHANGED: lifecycle chain extracted to helper methods
Client->>Evaluator: evaluate/evaluate_sync(cell, glbls)
Evaluator->>Evaluator: _setup_chain(cell, glbls)
loop For each lifecycle
alt Lifecycle returns Skip
Evaluator->>Evaluator: Record skip decision
else Lifecycle completes
Evaluator->>Evaluator: Append to completed list
end
end
alt Skip was returned
Evaluator->>Evaluator: value = skip.value
else No skip
Evaluator->>Executor: execute_cell / execute_cell_async(cell, glbls)
Executor-->>Evaluator: output value
end
Evaluator->>Evaluator: _teardown_chain(cell, glbls, completed, value, exception)
loop Reversed completed lifecycles
Evaluator->>Evaluator: teardown(cell, glbls, result)
end
Evaluator-->>Client: RunResult
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
612a55b to
6d9c8c1
Compare
## 📝 Summary <!-- If this PR closes any issues, list them here by number (e.g., Closes #123). Detail the specific changes made in this pull request. Explain the problem addressed and how it was resolved. If applicable, provide before and after comparisons, screenshots, or any relevant details to help reviewers understand the changes easily. --> When you run mo.sql with duckdb from a fresh engine, it prompts you to install `duckdb`, and `sqlglot`, and then after running, prompts to install polars/pandas. This now gets you to install all 3 packages from the start. <img width="1466" height="428" alt="image" src="https://github.com/user-attachments/assets/d5497301-0bb7-41aa-a0a3-baa2d16b5c28" /> ## 📋 Pre-Review Checklist <!-- These checks need to be completed before a PR is reviewed --> - [ ] For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on [Discord](https://marimo.io/discord?ref=pr), or the community [discussions](https://github.com/marimo-team/marimo/discussions) (Please provide a link if applicable). - [x] Any AI generated code has been reviewed line-by-line by the human PR author, who stands by it. - [x] Video or media evidence is provided for any visual changes (optional). <!-- PR is more likely to be merged if evidence is provided for changes made --> ## ✅ Merge Checklist - [x] I have read the [contributor guidelines](https://github.com/marimo-team/marimo/blob/main/CONTRIBUTING.md). - [ ] Documentation has been updated where applicable, including docstrings for API changes. - [x] Tests have been added for the changes made.
6d9c8c1 to
0500e7d
Compare
## 📝 Summary This fix is a followup to #9617 and #9616 and restores the truncated stacktrace to the user's notebook. --- Before fix: <img width="1086" height="405" alt="image" src="https://github.com/user-attachments/assets/39918a95-a4ec-4ca5-b701-d77fef42169f" /> After fix: <img width="1088" height="397" alt="image" src="https://github.com/user-attachments/assets/f73e98ba-7bfd-4559-bd34-ef0ea1515bbd" />
| if exc is None: | ||
| return None | ||
| if isinstance(exc, MarimoRuntimeException) and isinstance( | ||
| exc.__cause__, MarimoStopError |
There was a problem hiding this comment.
is there ever a plain MarimoStopError that is not wrapped? maybe we can be defensive?
| # Refresh the async decision with the caller's substitutions — | ||
| # an unsubstituted ancestor may have been async but isn't on | ||
| # this call's ancestor closure. | ||
| if by_kwargs.is_coroutine( |
There was a problem hiding this comment.
this is no longer cached. intentional?
## 📝 Summary <!-- If this PR closes any issues, list them here by number (e.g., Closes #123). Detail the specific changes made in this pull request. Explain the problem addressed and how it was resolved. If applicable, provide before and after comparisons, screenshots, or any relevant details to help reviewers understand the changes easily. --> Press S to open up speaker notes. Note that in kiosk mode, I've removed the filename as it seems uneccesary. https://github.com/user-attachments/assets/b83b918b-8482-49dd-990a-78fe3d0566d7 ## 📋 Pre-Review Checklist <!-- These checks need to be completed before a PR is reviewed --> - [x] For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on [Discord](https://marimo.io/discord?ref=pr), or the community [discussions](https://github.com/marimo-team/marimo/discussions) (Please provide a link if applicable). - [x] Any AI generated code has been reviewed line-by-line by the human PR author, who stands by it. - [x] Video or media evidence is provided for any visual changes (optional). <!-- PR is more likely to be merged if evidence is provided for changes made --> ## ✅ Merge Checklist - [x] I have read the [contributor guidelines](https://github.com/marimo-team/marimo/blob/main/CONTRIBUTING.md). - [x] Documentation has been updated where applicable, including docstrings for API changes. - [x] Tests have been added for the changes made. --------- Co-authored-by: Kiran Gadhave <14944083+kirangadhave@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
show cell index in dependency minimap <img width="651" height="350" alt="Screenshot 2026-05-20 at 5 06 55 PM" src="https://github.com/user-attachments/assets/d15b0545-f9fb-4b23-a7e6-5a4bb8d35fa7" /> <img width="655" height="360" alt="Screenshot 2026-05-20 at 5 06 49 PM" src="https://github.com/user-attachments/assets/3e86d2b0-b755-43d2-9a3a-8cc343973394" />
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
## 📝 Summary This fix is a followup to #9617 and #9616 and restores the truncated stacktrace to the user's notebook. --- Before fix: <img width="1086" height="405" alt="image" src="https://github.com/user-attachments/assets/39918a95-a4ec-4ca5-b701-d77fef42169f" /> After fix: <img width="1088" height="397" alt="image" src="https://github.com/user-attachments/assets/f73e98ba-7bfd-4559-bd34-ef0ea1515bbd" />
Adds behavior tests for four untested surfaces of the executor refactor,
and includes the one production-fix uncovered while planning.
Production fix (``marimo/_runtime/executor/executor.py``): let
``asyncio.CancelledError`` propagate unwrapped from
``DefaultExecutor.execute_cell{,_async}``. Wrapping it in
``MarimoRuntimeException`` defeated the bare-``CancelledError`` check
in ``Runner._finalize_run_result`` and prevented
``runner.interrupted`` from flipping on async SIGINT.
Tests:
- ``StrictLifecycle.setup`` ``Skip(result=...)`` path: 5 cases
covering undefined ref, ref-before-def, graph-resolved blamed cell,
private-var ``unmangle_local`` fallback, and the state invariant
that the Skip early-return must not mutate globals or stash a
backup (teardown then no-ops).
- ``unwrap_user_exception``: 6 cases, including the
``NameError.name is None`` guard. Construction note: bare
``NameError("name 'x' is not defined")`` does NOT set ``.name == 'x'``
— only interpreter-raised NameErrors do. Tests set ``.name`` explicitly.
- Plugin executor dispatch via ``Runner``: registers a sentinel
factory against ``_EXECUTOR_REGISTRY`` (fully isolated — both
``_plugins`` and ``names`` monkeypatched so installed third-party
entry points can't shadow), asserts the runner runs the sentinel.
- SIGINT and ``runner.interrupted``: deterministic ``_cancel_on_sigint``
tests via ``signal.signal``/``signal.getsignal`` monkeypatching
(real signals are unsafe in pytest — the wrapper chains to the
``previously installed`` handler, which in tests is pytest's, not
marimo's), plus the executor-level CancelledError-propagation test
and runner-level tests for both sync ``MarimoInterrupt`` and async
cancellation flipping the flag.
Resolves cubic's "this should be tested" comment on
``evaluator.py:97``.
- Inline ``Evaluator(...)`` at the two remaining ``build_evaluator`` call sites (``by_kwargs._new_evaluator`` and ``AppScriptRunner.__init__``) so the helper removal on ``dm/executor-impl`` doesn't leave dangling imports here. - Extract ``_strip_frame(e, count=1)`` util in ``executor.py`` for the executor-frame elision logic. Iterates ``count`` times and stops if the traceback runs out, never stripping the only frame we have — fixes the subtle issue where ``tb_next`` being ``None`` would set ``__traceback__`` to ``None`` outright. - ``_classify`` in ``by_kwargs`` now short-circuits on a bare ``MarimoStopError`` before the ``MarimoRuntimeException`` unwrap branch — any caller bypassing the default wrap (e.g. a custom ``Executor`` that raises ``MarimoStopError`` directly) still gets stop-control-flow handling.
3306d51 to
ed7a283
Compare
Markdown like `mo.md("")` previously produced a
broken `<img src="public/image.png">` reference in the exported
standalone
HTML — opening the file outside the notebook directory would 404 on
every
image. The static HTML export only inlined `./@file/` virtual files, not
relative `public/` paths.
This change adds `replace_public_files_with_data_uris` and runs it
during
`Exporter.export_as_html` (alongside the existing virtual-file pass) so
`<img>`, `<audio>`, `<video>`, and `<source>` `src` attributes pointing
at
the notebook's `public/` folder are embedded as base64 data URIs. The
same
10 MB inline limit used for virtual files applies; oversized files are
left as-is. Resolved paths are validated to stay inside the public
directory, rejecting `../` traversal and symlink escapes — matching the
runtime `serve_public_file` containment check.
## 📝 Summary Replaces `dict[str, Any]` in the executor pipeline with `MutableGlobals` (alias), and `Globals` (`Mapping[str, Any]`). Layering on top to prevent rebase churn
…lution (#9634) **This pull request was authored by a coding agent.** Fixes marimo-team/ci-agent#27. `test_required_dependencies` reads the live on-disk `pyproject.toml`. Under pytest-xdist, a leaking real `uv add` from a parallel test can append `test-package` mid-session (the `uv sync` step fails without rolling back the file write), flaking the snapshot read. Cache `pyproject.toml` contents at `tests/conftest.py` import time (worker startup, before any test runs) and expose them via a session-scoped `pyproject_text` fixture. ## Test plan - [x] `uv run --group test pytest tests/test_project_dependencies.py -v` - [x] `uv run --group test pytest tests/test_project_dependencies.py tests/_runtime/packages/ tests/_server/api/endpoints/test_packages.py -n auto`
**This pull request was authored by a coding agent.** Fixes #9628. With `auto_reload` set to `lazy` or `autorun`, every cell run was calling `ModuleReloader.check(sys.modules, reload=True)`, which iterates *all* of `sys.modules` and does `os.stat` on each entry. With ~1000 modules in scope (typical), that adds 16–80ms per cell — compounded across the dozen cells re-running on a UI interaction it becomes a >1s lag. This change adds an opt-in `skip_non_user_modules=True` flag on `ModuleReloader.check`. When set, stdlib and site-packages module names are recorded in a persistent skip set (classified by `sysconfig` prefixes) and short-circuited on subsequent calls. `AutoreloadManager.cell_scope` (the hot per-cell path) opts in. The background `ModuleWatcher` keeps the default behavior and continues to scan every module on its 1s loop, so edits inside an installed package are still detected — just at watcher latency rather than cell-entry latency. Editable installs (`pip install -e .`, `uv add --editable`) have `__file__` outside site-packages, so they are correctly classified as user code and reload with no latency change. ### Benchmark Driving `ModuleReloader.check()` directly, 200 iterations post-warmup. Issue-shaped workload: ~2.5k modules (heavy stdlib + numpy/pandas/etc.) + 5 user files in a tmp dir. | path | median | p95 | |------|-------:|----:| | before | 4.88 ms | 6.15 ms | | after | 0.91 ms | 1.01 ms | **~4 ms saved per cell run, 5.4× median speedup.** Scale curve (median µs, varying user-module count): | user mods | sys.modules | before | after | speedup | |----------:|------------:|-------:|------:|--------:| | 0 | 2514 | 5037 | 873 | 5.8× | | 5 | 2519 | 5245 | 802 | 6.5× | | 25 | 2539 | 6082 | 1693 | 3.6× | | 100 | 2614 | 8342 | 4421 | 1.9× | | 500 | 3014 | 12489 | 8398 | 1.5× | The win narrows as user-code grows, by design: the optimization only filters out non-user-code.
| async def evaluate_interruptible( | ||
| self, cell: CellImpl, glbls: MutableGlobals | ||
| ) -> RunResult: | ||
| """Await ``evaluate`` with SIGINT capture for coroutine cells. |
There was a problem hiding this comment.
Here and everywhere else, single backticks for code in docstrings. We don't write RST.
| Backs the ``Cell.run(**kwargs)`` public API. Walks the cell's ancestor | ||
| closure (minus any ancestor whose defs the caller substituted via | ||
| kwargs), runs them with a fresh globals dict, then runs the target cell. |
There was a problem hiding this comment.
It's not good practice to document consumers of an API. It's action at a distance. If Cell.run were changed to no longer use this, you wouldn't think to change this docstring.
There was a problem hiding this comment.
Cleaned up comments!
## 📝 Summary <!-- If this PR closes any issues, list them here by number (e.g., Closes #123). Detail the specific changes made in this pull request. Explain the problem addressed and how it was resolved. If applicable, provide before and after comparisons, screenshots, or any relevant details to help reviewers understand the changes easily. --> 1. Fix playwright test 2. For a notebook with very few cells, we didn't stretch the slide area fully. This fixes that. See the snapshot picture for reference. ## 📋 Pre-Review Checklist <!-- These checks need to be completed before a PR is reviewed --> - [ ] For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on [Discord](https://marimo.io/discord?ref=pr), or the community [discussions](https://github.com/marimo-team/marimo/discussions) (Please provide a link if applicable). - [x] Any AI generated code has been reviewed line-by-line by the human PR author, who stands by it. - [ ] Video or media evidence is provided for any visual changes (optional). <!-- PR is more likely to be merged if evidence is provided for changes made --> ## ✅ Merge Checklist - [x] I have read the [contributor guidelines](https://github.com/marimo-team/marimo/blob/main/CONTRIBUTING.md). - [ ] Documentation has been updated where applicable, including docstrings for API changes. - [x] Tests have been added for the changes made.
…ST) (#9645) Our docstrings are in Markdown, not RST. This PR adds a precommit that enforces markdown-style inline code (single backticks), and a note in AGENTS.md telling agents about this convention. The PR also fixes all offending instances of double backticks. This convention matters because it affects how docstrings are parsed/rendered in our doc. It also matters for just internal style consistency. --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
## 📝 Summary <!-- If this PR closes any issues, list them here by number (e.g., Closes #123). Detail the specific changes made in this pull request. Explain the problem addressed and how it was resolved. If applicable, provide before and after comparisons, screenshots, or any relevant details to help reviewers understand the changes easily. --> - Pressing Enter with no input in marimo's stdin box was silently swallowed by the frontend, so any CLI calling `input("Press Enter to continue:")` (e.g. `logfire`, `click.prompt`, `rich.prompt`, `pdb`) would hang forever. - Once unblocked, a second bug surfaced: `builtins.input()` raised `EOFError` on blank submissions because `ThreadSafeStdin.readline()` returned `""` — which CPython's `input()` interprets as EOF rather than a blank line. ### Fix **Frontend** (`ConsoleOutput.tsx`) - `StdInput` now submits on Enter even when empty (history skip preserved). - `StdInputWithResponse` renders past responses with a `>` chevron, sky-11 color, and an `(empty)` placeholder so user-typed responses are visually distinct from stdout — even when the prompt half is empty (which happens when CLIs `print()` the prompt themselves before calling `input()`). <img width="501" height="191" alt="image" src="https://github.com/user-attachments/assets/59999aad-09fe-4174-9408-b009981130be" /> **Backend** (`marimo/_messaging/types.py`, `streams.py`, `_pyodide/streams.py`) - `Stdin.readline()` now appends `"\n"` so blank submissions surface as `"\n"` rather than `""`. `readlines()` simplified to `[readline()]`. - Lifted `readline`/`readlines` to the `Stdin` base class — both subclasses had identical implementations. - `_readline_with_prompt` unchanged: still returns the bare response, so `input_override` continues to satisfy Python's `input()` contract. ## 📋 Pre-Review Checklist <!-- These checks need to be completed before a PR is reviewed --> - [x] For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on [Discord](https://marimo.io/discord?ref=pr), or the community [discussions](https://github.com/marimo-team/marimo/discussions) (Please provide a link if applicable). - [x] Any AI generated code has been reviewed line-by-line by the human PR author, who stands by it. - [x] Video or media evidence is provided for any visual changes (optional). <!-- PR is more likely to be merged if evidence is provided for changes made --> ## ✅ Merge Checklist - [x] I have read the [contributor guidelines](https://github.com/marimo-team/marimo/blob/main/CONTRIBUTING.md). - [ ] Documentation has been updated where applicable, including docstrings for API changes. - [x] Tests have been added for the changes made.
…s-origin iframes (#9649)
Removed the section on adding a formatter to the marimo repo. It's better if folks make their own viewers for their own objects.
|
Woops. Will rebase, bad merge |
There was a problem hiding this comment.
1 issue found across 15 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="marimo/_runtime/app/script_runner.py">
<violation number="1" location="marimo/_runtime/app/script_runner.py:163">
P3: Comment has a grammatical issue: "exceptions are expected to be wrapper" should be "...to be wrapped" or "...to be a wrapper". The double period at the end is also likely a typo for a single period.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| return | ||
| if not isinstance(exc, BaseException): | ||
| # Defensive check descendants, since all exceptions are expected to | ||
| # be wrapper.. |
There was a problem hiding this comment.
P3: Comment has a grammatical issue: "exceptions are expected to be wrapper" should be "...to be wrapped" or "...to be a wrapper". The double period at the end is also likely a typo for a single period.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At marimo/_runtime/app/script_runner.py, line 163:
<comment>Comment has a grammatical issue: "exceptions are expected to be wrapper" should be "...to be wrapped" or "...to be a wrapper". The double period at the end is also likely a typo for a single period.</comment>
<file context>
@@ -153,22 +153,14 @@ def _handle_run_result(
- # treat defensively by recording the output and cancelling
- # descendants.
+ # Defensive check descendants, since all exceptions are expected to
+ # be wrapper..
outputs[cid] = result.output
self._scheduler.cancel(cid)
</file context>
| # be wrapper.. | |
| # be wrapped. |
📝 Summary
This PR cleans up
dataflow::Runnerinto functions that directly utilize the evaluator pipeline introduced in #9616.Additionally, it consolidates
AppScriptRunnerto use this logic as well.A note, this refactor prevents AppScriptRunner from classifying
mo.stopas an exception. The previous behavior would throw and cause a total script stop- the revised behavior (and desired), is that the dependency tree is cancelled while the rest of the notebook can compute.